feat(postgres): allow running multiple statements in no tx migrations #3694
Conversation
There was a problem hiding this comment.
See #3574 for previous discussion.
I would prefer if the style matched the existing -- no-transaction annotation.
To make the annotation more selective, I would require it to match on a full line. That way someone can write a comment talking about it without accidentally introducing a statement break where they didn't intend to.
I also don't see much benefit in namespacing the annotations. I don't see much harm, but I also don't see much benefit.
Since the existing annotation isn't namespaced, adding a namespaced annotation (as well as using a different comment style) would just be confusing to the user.
My argument to add namespacing was mostly about making it clearer to the user that the annotation is in-fact an annotation. ( Also it implicitly communicates that the annotation comes from The last point is that I am reluctant about whitespaces in user interfaces, because they are far too easy to screw up and might lead to hard to diagnose errors. Like when writing an annotation a user might accidentally add an tab character instead of space, or have two spaces instead of one. That is especially the case when dealing with a system that has relatively little insight into what is happening as this.
But yeah. In the end all those points don't have too much impact and just a few things I tend to be cautious of. |
|
@abonander any other changes other than renaming and matching the full line? Do you generally think this implementation is more viable? |
|
@abonander I've renamed as per your request and made it only match a full line, including adding a test for the latter |
| CREATE INDEX CONCURRENTLY test_table_x_idx ON test_table (x); | ||
| -- split-migration | ||
| INSERT INTO test_table (x) VALUES (1); | ||
| -- prove that you can have a comment that won't split -- split-migration DROP TABLE does_not_exist; |
There was a problem hiding this comment.
I confirmed that if I remove the \ns from the string split this gets split such that DROP TABLE does_not_exist; becomes runnable SQL that then fails to run.
| let sql = migration.sql.trim(); | ||
| // note: this would _not_ match the split if the file starts with `-- split-migration` | ||
| // because it requires a new line prefix, but that doesn't really make sense anyway so it's fine | ||
| let split_migrations = sql.split("\n-- split-migration\n"); |
There was a problem hiding this comment.
Bikeshedding: I don't think -migration adds any information here. If I encountered this without context while reading through a migration file and didn't already know what it did, I'd just have a lot more questions:
- How is it splitting the migration? Is it generating a separate record in the migrations table?
- What does splitting the migration mean?
- Why would you want to split the migration?
I think we could choose a much more self-descriptive name here. I suggested -- statement-break in the original issue but I'm not sure that's helpful, either. "Aren't statements already broken up by semicolons?" I would ask.
Here's some ideas:
-- split-execute-- execute-separately-- transaction-break-- autocommit-break- I like the idea of working "autocommit" in there somewhere because it could be helpful to remind the user why this exists.
-- split-autocommit-- transactional-ddl-split
There was a problem hiding this comment.
I'm happy with any of them, feel free to pick your favorite and I'll edit the PR 😄
There was a problem hiding this comment.
Hey @abonander any thoughts on which marker we should choose? Of those options I personally like -- transaction-break but again no strong opinion
There was a problem hiding this comment.
Yeah, transaction-break seems to be the front-runner, but my biggest concern is someone might look at a migration file and see something like this:
-- no-transaction
Statement;
Statement;
Statement;
-- transaction-break
Command;
-- transaction-break
Statement;and be really confused. "Wtf? I thought this was already not executing in a transaction? What does -- transaction-break mean if it's not supposed to be in a transaction in the first place?"
However, I'm now realizing that this functionality actually can supercede -- no-transaction as long as we make sure to handle the migration as if it started with -- no-transaction.
This actually has great potential value to the user because we can execute everything before the first -- transaction-break in a transaction block, so if there's an error in that part of the migration, it can be completely rolled back and the user isn't stuck with a partially applied migration.
And also, if the only statement in the migration is one that can't happen in a transaction, -- no-transaction and -- transaction-break are effectively equivalent.
There is probably still value in keeping -- no-transaction in that situation because I believe there's "Postgres-compatible" third-party databases that don't support any DDL in a transaction, and -- no-transaction kind of makes more sense as a whole-file flag than -- transaction-break.
Alternatively, we could just roll this functionality into -- no-transaction since it's essentially a superset of its behavior. But I think seeing multiple -- no-transaction lines in a migration would also be confusing; it'd be like, "did it not get the message the first time?"
Maybe... roll it together, support -- transaction-break as an alias of -- no-transaction and leave it up to the user as a stylistic choice? But it's probably better to just pick one label and stick with it.
There was a problem hiding this comment.
Eh, I would probably leave -- no-transaction as it is. But use of -- transaction-break would make it optional.
Closes #3693